refactor(billing): require wallet address in public wallet API responses - #3521
Open
baktun14 wants to merge 5 commits into
Open
refactor(billing): require wallet address in public wallet API responses#3521baktun14 wants to merge 5 commits into
baktun14 wants to merge 5 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughWallet initialization now guarantees initialized public wallet data, frontend hooks handle wallet presence independently of address presence, usage history dates are materialized during parsing, and API schemas document updated wallet, trial, and idempotency contracts. ChangesWallet initialization and API contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3521 +/- ##
==========================================
- Coverage 74.18% 73.22% -0.97%
==========================================
Files 1142 1052 -90
Lines 29721 27371 -2350
Branches 7447 6979 -468
==========================================
- Hits 22049 20042 -2007
+ Misses 6771 6459 -312
+ Partials 901 870 -31
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports for wallet aliases.
UserWalletOutput,UserWalletPublicOutput, andWalletInitializedare type-only symbols, whileUserWalletRepositoryis a runtime dependency. Split or annotate these imports to satisfy the shared TypeScript import rule and avoid runtime import issues.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts#L6-L6: mark the wallet aliases as type-only while keepingUserWalletRepositoryas a value import.apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts#L6-L6: apply the same type-only import treatment.As per coding guidelines, TypeScript imports must use consistent type imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts` at line 6, Update the imports in apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts#L6-L6 and apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts#L6-L6 so UserWalletOutput, UserWalletPublicOutput, and WalletInitialized use type-only imports, while UserWalletRepository remains a runtime import.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts`:
- Around line 87-94: Update `#ensureWalletVia` to perform the wallet address
mutation through the authorization-scoped repository argument by replacing
this.userWalletRepository.updateById with repository.updateById, while
preserving the existing update parameters and returned wallet behavior.
In `@apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts`:
- Around line 22-24: Update the wallet filter in the wallet-reader mapping flow
to validate that address is present and non-empty before asserting
WalletInitialized; reject undefined, null, and empty or whitespace-only values
while preserving the activatedAt requirement and existing toPublic mapping.
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts`:
- Around line 19-20: Change the WalletInitialized import in the deployment
reader service to a type-only import, while keeping WalletReaderService as a
regular import.
In `@apps/api/swagger/openapi.json`:
- Line 1790: Remove the generated static default value for dynamic endDate at
apps/api/swagger/openapi.json lines 1790-1790 and 1916-1916, omitting the
default property entirely at both sites so the OpenAPI schema does not contain a
date that becomes stale.
- Line 5: Update the OpenAPI server URL in the generator configuration to the
intended public API URL instead of localhost, then regenerate the specification
so Swagger UI and generated clients use the deployed server.
---
Nitpick comments:
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts`:
- Line 6: Update the imports in
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts#L6-L6
and apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts#L6-L6
so UserWalletOutput, UserWalletPublicOutput, and WalletInitialized use type-only
imports, while UserWalletRepository remains a runtime import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cee7dad8-2ed7-4893-a71b-d7722cb62820
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
apps/api/src/billing/http-schemas/wallet.schema.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/billing/services/wallet-reader/wallet-reader.service.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/deployment/services/lease/lease.service.spec.tsapps/api/swagger/openapi.jsonapps/api/test/seeders/user-wallet.seeder.tsapps/deploy-web/src/hooks/useEnsureTrialStarted.spec.tsapps/deploy-web/src/hooks/useEnsureTrialStarted.tsapps/deploy-web/src/hooks/useManagedWallet.spec.tsxapps/deploy-web/src/hooks/useManagedWallet.tspackages/console-api-types/src/schema.d.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/hooks/useEnsureTrialStarted.spec.ts
- merge duplicated @src/billing/repositories import in deployment-writer - flatten managed-wallet storage-sync effect to an early return + ternary - revert accidental openapi regeneration artifacts (localhost server url, today's date defaults, dropped trailing newline)
… guard Addresses review feedback on the wallet-address contract: - #ensureWalletVia now mutates through the CASL-scoped `repository` argument instead of the unscoped `this.userWalletRepository`, so the get/check/mutate sequence stays authorization-scoped during trial init. - getWallets filters activated wallets with a truthiness check on `address` so an empty-string address is also excluded and the `WalletInitialized` predicate holds. - Use type-only imports for the wallet type aliases and WalletInitialized.
…enAPI spec The usage-history query schema defaulted `endDate` with `.default(() => new Date()...)`. The OpenAPI generator evaluates that at generation time and freezes a static date into the spec, which then goes stale and misleads generated clients into sending an old default date. Move the "default to today" logic into the schema transform (next to the existing startDate-from-endDate derivation) so runtime behavior is unchanged but the generated spec no longer carries a static default. Regenerate openapi.json, update the docs snapshot, and drop the now-redundant non-null assertions in the router and refine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Closes CON-745
CON-744 (#3512, #3513, deployed) guarantees that every wallet returned by the public API has an address:
GET /v1/walletsonly returns activated wallets (activation happens strictly after address derivation), andPOST /v1/start-trialresponds only after the address is derived. The loosenullablecontract now only hides bugs and forces null-tolerant handling in API consumers.What
apps/api — declare
address(plusid,userId,createdAt, which are DB-non-null already) as required in the wallet response schema:WalletOutputSchema: drop.nullable()on the four fields;createdAtswitchesz.coerce.date()→z.date()because coercion makes the OpenAPI generator'sisNullable()probe succeed onnull(new Date(null)is valid), which kept a phantomnullable: truein the spec. Response schemas are compile-time/docs only — nothing parses them at runtime.toPublicnow requiresWalletInitialized(moved to the repository fromwallet-reader.service.ts),WalletReaderService.getWalletsfilters with a type predicate (which also silently excludes any pathological activated-but-addressless legacy row instead of 500ing), andWalletInitializerService.#ensureWalletViareturns the narrowed type its JSDoc already promised.createInitializedUserWalletseeder for fixtures that mock the narrowed contract.Generated artifacts — docs snapshot,
swagger/openapi.json, and@akashnetwork/console-api-typesregenerated. Note: the checked-in spec was stale (predates #3513 and the idempotent top-up work), so the regen also drops the long-removed202from/v1/start-trial, adds its409, and picks upidempotencyKeyon checkout — incidental drift, not introduced by this PR.apps/deploy-web — drop the null-address tolerance that CON-744 shipped as a temporary bridge:
useManagedWalletand remove its now-impossible null-address specs.useEnsureTrialStarted:isWalletReadyis now simply "a wallet came back from the API" (!!wallet).Verified end-to-end against a locally booted API: an activated wallet returns a non-null address, a pre-activation wallet is excluded from
GET /v1/wallets, and the served OpenAPI doc marks all four fields required (only the intentionally nullable 3DS fields remain nullable).Summary by CodeRabbit
idempotencyKeysupport for Stripe transaction confirmation requests.409response for/v1/start-trialwhen trial provisioning is already in progress.id,userId,address, andcreatedAtwhere applicable.endDateand derivingstartDatewhen needed.